Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Selectively skip and fix tests after PR#212 #225

Closed
wants to merge 23 commits into from
Closed

Selectively skip and fix tests after PR#212 #225

wants to merge 23 commits into from

Conversation

dliappis
Copy link
Contributor

@dliappis dliappis commented Jan 30, 2024 •
edited
Loading

PR #212 made some changes on main that require the presence of MANIFEST_URL. However, the pipeline is still configured to run the same script when a PR (from an upstream branch) is raised and it currently fails.

This commit skips tests when triggered from another pipeline (unified release), and allows running tests when triggered by PR creation, or branch pushed by picking up a suitable MANIFEST_URL.

@dliappis dliappis added the bug Something isn't working label Jan 30, 2024
@dliappis dliappis self-assigned this Jan 30, 2024
@dliappis dliappis requested a review from a team as a code owner January 30, 2024 14:36
@dliappis
Copy link
Contributor Author

dliappis commented Jan 30, 2024 •
edited
Loading

This is ready for review. Because of the multiple ways that this pipeline may run -- via unified release trigger, via branch commits or pr commits -- we need to support a different way of running depending on the occasion:

  1. If we open the PR it (e.g. the latest commit here: https://buildkite.com/elastic/elastic-stack-installers/builds/3646) it looks like:

    image

  2. If the pipeline gets triggered by the unified release it should run as before (and allow to go through the DRA publish).

  3. Special handling is done for staging builds:

    • Unified release may trigger the pipeline but only for non main branches and specifically if they target [0-9]+.[0-9]+ matching branches. Tests are skipped.
    • PRs are only allowed to run the steps (including tests) if they target non-main branch (the reason is that there is no https://staging.elastic.co/latest/main.json, so we can't deduce a valid MANIFEST_URL)

  4. We've removed runs based on push events -- e.g. cutting a new branch from main -- (which used to be allowed before); this is because it complicates unnecessarily the conditionals. This use case (and more) will be covered in a follow up PR, through a scheduled job that runs at regular intervals.

Example run: https://buildkite.com/elastic/elastic-stack-installers/builds/3718#018d6acc-6f2e-43b8-b8ea-3514dd9f919e using this PR

@dliappis
Copy link
Contributor Author

@strawgate to implement what we discussed in do you think it would it be enough to just present a variable here (which is the conditional that covers the case that our job/scripts gets triggered from the unified release) and conditionally run tests in

elastic-stack-installers/.buildkite/scripts/build.ps1

Line 185 in d1bdb21

& (Join-Path $PSScriptRoot "test.ps1")
only if the var is present?

@strawgate
Copy link
Contributor

@strawgate to implement what we discussed in do you think it would it be enough to just present a variable here (which is the conditional that covers the case that our job/scripts gets triggered from the unified release) and conditionally run tests in

elastic-stack-installers/.buildkite/scripts/build.ps1

Line 185 in d1bdb21

& (Join-Path $PSScriptRoot "test.ps1")

only if the var is present?

Yes!

dliappis added a commit that referenced this pull request Jan 31, 2024
@dliappis
Don't run tests when triggered by unified release
b1c05ba 
We don't want to run tests when we are called from the unified release.
This commit implemented the suggestion in #225 (comment)
@dliappis
Copy link
Contributor Author

@strawgate to implement what we discussed in do you think it would it be enough to just present a variable here (which is the conditional that covers the case that our job/scripts gets triggered from the unified release) and conditionally run tests in

elastic-stack-installers/.buildkite/scripts/build.ps1

Line 185 in d1bdb21

& (Join-Path $PSScriptRoot "test.ps1")

only if the var is present?

Yes!

@strawgate this has been implemented in b1c05ba . Please take a look.

@dliappis dliappis changed the title Fix PR tests after PR#212 Selectively skip and fix tests after PR#212 Jan 31, 2024
@dliappis
Fix PR tests after PR#212
0d4fc88 
PR#212 made some changes on `main` and `8.12` that require the presence
of MANIFEST_URL. However, the pipeline is still configured to run
the same script when a PR (from an upstream branch) is raised and
it currently fails.

This commit fixes PR tests by picking up a suitable MANIFEST_URL.
@dliappis
PR comment
1585419
@dliappis
Fix tests when run via branch push
2b753cf
@dliappis
typo
f6f0097
@dliappis
fix test for branch push
ce3352c
@dliappis
revert
ea4f937
@dliappis
Don't run tests when triggered by unified release
ac39448 
We don't want to run tests when we are called from the unified release.
This commit implemented the suggestion in #225 (comment)
@dliappis
Improvements
ed214a7
@dliappis dliappis closed this Feb 2, 2024
@dliappis
Copy link
Contributor Author

dliappis commented Feb 2, 2024

close #225 in favor of #235

@dliappis dliappis mentioned this pull request Feb 7, 2024
Closed
@dliappis dliappis deleted the fix-tests branch February 19, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amitkanfer amitkanfer amitkanfer left review comments

@strawgate strawgate Awaiting requested review from strawgate

@DaveSys911 DaveSys911 Awaiting requested review from DaveSys911

Assignees

@dliappis dliappis

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dliappis @strawgate @amitkanfer